Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preserve property order in environment editor #1497

Merged
merged 24 commits into from
May 29, 2019
Merged

Preserve property order in environment editor #1497

merged 24 commits into from
May 29, 2019

Conversation

develohpanda
Copy link
Contributor

@develohpanda develohpanda commented May 12, 2019 •
edited
Loading

Closes #1046.

image

Added an option to keep property order by consuming json-order. This is opt-in, thus disabled by default. This behavior is available to both the workspace environment editor, and the request group override environment editor.

Field names cannot begin by $ or contain a . (as part of the nedb documentation).

As a result, the map generation uses & as the prefix and ~| as the separator. These two options are parameters in the parse and stringify methods. An example map is shown below.

const map = {
  "&": ["a"],
  "&~|a": ["z", "y"],
  "&~|a~|z": ["b"],
  "&~|a~|y~|2": ["d", "c"]
}

@develohpanda
Copy link
Contributor Author

develohpanda commented May 12, 2019 •
edited
Loading

I'm not sure I added the dependency correctly - should json-order be under packedDependencies or dependencies?

@develohpanda develohpanda changed the title 1046 - sort environment variables Preserve property order in environment editor May 12, 2019
@gschier
Copy link
Contributor

gschier commented May 15, 2019

I'm not sure I added the dependency correctly - should json-order be under packedDependencies or dependencies?

It should be in both. packedDependencies is a special insomnia-specific property that the build script uses to figure out whether the dependency should be bundled in the Webpack output or left as an external node module. In general, most should be bundled, but some modules don't work like that so need to be left alone.

@develohpanda
Copy link
Contributor Author

develohpanda commented May 16, 2019 •
edited
Loading

Hmmm - I'm not sure why the CI builds are failing. I added to the packed dep but it still failed. Surely I've missed a step somewhere while adding the package 🤔

Works locally:
image

@gschier
Copy link
Contributor

gschier commented May 21, 2019 •
edited
Loading

Ah, @develohpanda it's because you need to add a Flow definition to the flow-typed folder. I wish there were a way to have a better error message when that happens.

Here's a good example of one to take a look at:

https://github.com/getinsomnia/insomnia/blob/639d0cc33b0d455f43f449da1cbbe42dffa443cc/packages/insomnia-app/flow-typed/mkdirp.js

gschier
gschier suggested changes May 21, 2019
View reviewed changes
Copy link
Contributor

@gschier gschier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looking great! Just a couple small comments and then one about whether or not we should have the checkbox to maintain order.

packages/insomnia-app/app/models/environment.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/models/request-group.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/components/editors/environment-editor.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/components/editors/environment-editor.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/ui/components/modals/environment-edit-modal.js
@@ -30,10 +30,20 @@ class EnvironmentEditModal extends PureComponent {
return;
}

const environment = this._envEditor.getValue();
let patch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you included the try/catch when there wasn't one before. Did you find an error case that wasn't handled?

packages/insomnia-app/app/ui/components/modals/workspace-environments-edit-modal.js
@@ -342,9 +343,13 @@ class WorkspaceEnvironmentsEditModal extends React.PureComponent<Props, State> {
return;
}

let data;
let patch;
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Same thing here. I just checked the getValue function and didn't realize it was doing JSON parsing. Makes sense now 👍

packages/insomnia-app/app/ui/components/modals/workspace-environments-edit-modal.js Show resolved Hide resolved
packages/insomnia-app/app/ui/css/layout/base.less Outdated Show resolved Hide resolved
@develohpanda
Copy link
Contributor Author

@gschier ready for review again :)

gschier
gschier approved these changes May 29, 2019
View reviewed changes
Copy link
Contributor

@gschier gschier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just gave it a spin locally and it works great 😄

@gschier gschier merged commit 24a2e75 into Kong:develop May 29, 2019
@develohpanda develohpanda deleted the feature/1046-sort-env-variables branch May 31, 2019 10:46
@gschier gschier mentioned this pull request Dec 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gschier gschier gschier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Sort environment variables
2 participants
@develohpanda @gschier